GH-3716: Fix data corruption in ByteBufferBackedBinary.getBytes() for non-array-backed buffers - #3717
Conversation
…y-backed buffers FixedLenByteArrayPlainValuesReader hands out Binary values that all share one page-wide ByteBuffer, advancing its live position on every readBytes() call. getBytes() and toStringUsingUTF8() on the non-array-backed path called value.limit(offset + length) directly on that shared buffer before capturing position. ByteBuffer.limit() clamps position down whenever position > newLimit, so calling getBytes() on an earlier value after later values have already advanced the buffer permanently rewinds its live position -- corrupting every readBytes() call that follows. This surfaces as data corruption when reading a repeated (LIST) FIXED_LEN_BYTE_ARRAY column with 2+ elements per row across 2+ rows: record assembly stores each Binary and only materializes it once a full row is built, which is exactly the lazy-after-later-value pattern that triggers the clamp. Each subsequent row reads back the previous row's last-written element instead of its own. Fix both methods to duplicate() the buffer before adjusting its position/limit, so the shared buffer's own position is never mutated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
yimingli-vmware
changed the title
yimingli-vmware
changed the title
yimingli-vmware
changed the title
|
Nice catch. +1 (non-binding). I confirmed the new test code fails under 1.18.0 |
Hi @dossett, thanks for the confirmation. I wish we could have a patch release for addressing this issue soon, since we are blocked by the Jackson CVEs which are resolved in 1.18.0. |
|
Out of curiosity, was this issue latent in the codebase, or was it introduced around the v1.18.0 changes?" |
|
Based on my analysis (with Codex) it was introduced in 1.18.0, specifically in #3565. I confirmed that 1.18.0 with that PR reverted will pass the new tests. |
|
@dossett Got it, thanks! So the bug in |
|
That's almost a philosophical question about what constitutes a bug :-) But there's definitely some truth to that. |
3 participants
Rationale for this change
Fixes #3716.
FixedLenByteArrayPlainValuesReaderhands outBinaryvalues that allshare one page-wide
ByteBuffer, advancing its live position on everyreadBytes()call.Binary.ByteBufferBackedBinary.getBytes()andtoStringUsingUTF8()(non-array-backed branch) calledvalue.limit(offset + length)directly on that same shared buffer beforecapturing
value.position(). SinceByteBuffer.limit()clampspositiondown whenever
position > newLimit, callinggetBytes()on an earliervalue after later values have already advanced the buffer permanently
rewinds the buffer's live position -- corrupting every
readBytes()callthat follows.
This surfaces as data corruption when reading a repeated (
LIST)FIXED_LEN_BYTE_ARRAYcolumn with 2+ elements per row across 2+ rows:record assembly stores each
Binaryand only materializes it once a fullrow/group has been built, which is exactly the lazy-after-later-value
pattern that triggers the clamp. Each subsequent row reads back the
previous row's last-written element instead of its own (see #3716 for a
minimal standalone repro).
What changes are included in this PR?
Binary.ByteBufferBackedBinary.getBytes()and.toStringUsingUTF8()nowduplicate()the buffer before adjusting position/limit, so the sharedbuffer's own position is never mutated.
TestFixedLenByteArrayPlainValuesWriterReaderthat read values out ofthe order they were materialized, matching the lazy-consumption pattern
from record assembly, and fail against unpatched 1.18.0.
Are these changes tested?
Yes -- two new tests
(
testLazyGetBytesDoesNotCorruptSubsequentReadsDirectBuffer,testLazyToStringUsingUTF8DoesNotCorruptSubsequentReadsDirectBuffer) inparquet-column/src/test/java/org/apache/parquet/column/values/plain/TestFixedLenByteArrayPlainValuesWriterReader.javafail on the unpatched code and pass with this fix. Also verified against
the full-file-roundtrip repro from #3716.
Are there any user-facing changes?
No API changes. This fixes a silent data-corruption bug introduced in
1.18.0; no user-facing behavior changes other than correct results.